Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MDEV-35948: Test OG replicate_*s with long lists #3795

Open
wants to merge 1 commit into
base: 10.5
Choose a base branch
from

Conversation

ParadoxV5
Copy link
Contributor

@ParadoxV5 ParadoxV5 commented Jan 28, 2025

Description

Assert that 1st-gen replicate_* filter variables display their input – including long but reasonable lists – correctly (without truncation) in

  • direct SELECT
  • [semi-new] INFORMATION_SCHEMA.GLOBAL_VARIABLES.VARIABLE_VALUE
  • [new] SHOW REPLICA STATUS

This tests the fields affected by MDEV-35693 (an 11.6 oversignt).

Also test the input DEFAULT as part of this edge-case testing.

Release Notes

N/A – This is a dev task.

How can this PR be tested?

run the modified files

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@@ -32,11 +36,23 @@ SET @@GLOBAL.replicate_wild_ignore_table="test.t, t2";
--error ER_WRONG_ARGUMENTS
SET @@GLOBAL.replicate_wild_ignore_table="test.,t1";

Copy link
Contributor Author

@ParadoxV5 ParadoxV5 Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over-max-sized identifiers don’t cause errors. Possible reasons:

  • 🐞
  • It’s the user’s problem that they was filtering for/against impossible names.
  • I made junior dev mistakes.
Suggested change
SET @name= REPEAT("t", 64);
SET @@GLOBAL.replicate_wild_ignore_table= CONCAT(@name, ".t");
#--error ER_WRONG_ARGUMENTS?
SET @@GLOBAL.replicate_wild_ignore_table= CONCAT(@name, "1.t");
SET @@GLOBAL.replicate_wild_ignore_table= CONCAT("t.", @name);
#--error ER_WRONG_ARGUMENTS?
SET @@GLOBAL.replicate_wild_ignore_table= CONCAT("t.", @name, "1");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think that size of each identifier is ever validated (quickly looking into sys_vars.cc, e.g. static Sys_var_rpl_filter Sys_replicate_wild_ignore_table). Perhaps that would be a good validation to add 😇 (in due time)

Comment on lines +42 to +43
SELECT GROUP_CONCAT(CONCAT("database_name.long_table_name_", seq) SEPARATOR ",")
INTO @name FROM seq_1_to_8;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MariaDB is awesome 😆

--source include/have_sequence.inc
# have show_slave_status
--source include/have_log_bin.inc
CHANGE MASTER TO master_host='example.com';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the time was spent on sys_vars.replicate_wild_ignore_table_basic (the other files simply modified from copy-pastes), and about half were spend working around how SHOW REPLICA STATUS is empty without CHANGE MASTER TO 😵 the hard way.

Copy link
Contributor

@bnestere bnestere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only had a quick moment today to look over the tests, generally, they look good! I only left a few small notes.

Also, as to the PR plan, I'd say these should go in with PR #3772, and then once that makes it into 11.7, we can backport it and extend the tests for the rest of the SHOW SLAVE STATUS variables.

mysql-test/suite/sys_vars/t/replicate_do_db_basic.test Outdated Show resolved Hide resolved
mysql-test/suite/sys_vars/t/replicate_do_db_basic.test Outdated Show resolved Hide resolved
@@ -32,11 +36,23 @@ SET @@GLOBAL.replicate_wild_ignore_table="test.t, t2";
--error ER_WRONG_ARGUMENTS
SET @@GLOBAL.replicate_wild_ignore_table="test.,t1";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think that size of each identifier is ever validated (quickly looking into sys_vars.cc, e.g. static Sys_var_rpl_filter Sys_replicate_wild_ignore_table). Perhaps that would be a good validation to add 😇 (in due time)

Assert that 1st-gen `replicate_*` filter variables display their input –
including long but reasonable lists – correctly (without truncation) in
* direct SELECT
* [semi-new] INFORMATION_SCHEMA.GLOBAL_VARIABLES.VARIABLE_VALUE
* [new] SHOW REPLICA STATUS

This tests the fields affected by MDEV-35693 (an 11.6 oversignt).

Also test the input `DEFAULT` as part of this edge-case testing.
@ParadoxV5 ParadoxV5 force-pushed the mdev-35948-replicate branch from dc590a4 to 3a7cbfa Compare January 30, 2025 22:33
@ParadoxV5
Copy link
Contributor Author

ParadoxV5 commented Jan 30, 2025

Amend + Rebase

diff --git a/mysql-test/suite/sys_vars/t/replicate_do_db_basic.test b/mysql-test/suite/sys_vars/t/replicate_do_db_basic.test
index a99429fe9a..4b72d14d6a 100644
--- a/mysql-test/suite/sys_vars/t/replicate_do_db_basic.test
+++ b/mysql-test/suite/sys_vars/t/replicate_do_db_basic.test
@@ -1,7 +1,7 @@
 --source include/have_sequence.inc
 # have show_slave_status
---source include/have_log_bin.inc
-CHANGE MASTER TO master_host='example.com';
+--source include/not_embedded.inc
+CHANGE MASTER TO master_host='127.0.0.1', master_user='root';
 --let $status_items= Replicate_Do_DB

 --echo #
@@ -58,5 +58,4 @@ SET @@GLOBAL.replicate_do_db=DEFAULT;
 SELECT @@GLOBAL.replicate_do_db;

 --echo # Cleanup.
-RESET REPLICA ALL;
 SET @@GLOBAL.replicate_do_db = @save_replicate_do_db;

CHANGE MASTER TO is simply required for some reason, even if it’s changing to the same configs.

ParadoxV5 added a commit that referenced this pull request Jan 31, 2025
Changes should have accompanying tests.

Assert that 1st-gen `replicate_*` filter variables display long but
reasonable lists correctly (without truncation) in SHOW REPLICA STATUS.
See that PR for details
ParadoxV5 added a commit that referenced this pull request Jan 31, 2025
Resize the types and widths of SHOW REPLICA STATUS
(technically `INFORMATION_SCHEMA.SLAVE_STATUS`)
columns to better match their possible values

In case of intentionally but absurdly long lists,
text columns that list an uncapped number of elements
have expanded to accept as many bytes as we could support.

Particularly, the first-gen `Replicate_` filters were
incorrectly typed as singlular `Name()`s during MDEV-33526.
Under `Name`s’ 64-char limit, they could overflow
(read: truncate) even before their lengths got absurd.

In response to `‘MAX_SLAVE_ERRMSG’ was not declared in this scope` in
Embedded builds, a new `#ifdef HAVE_REPLICATION` guard wraps
`slave_status_info` to skip this unused data in Replication-less builds.

For testing, this commit forward-ports a modified cherry-pick of #3795
(the latter targets our oldest maintained LTS as part of MDEV-35948).

> Assert that 1st-gen `replicate_*` filter variables display
> their input – including long but reasonable lists –
> correctly (without truncation) in
> * direct SELECT
> * [semi-new] INFORMATION_SCHEMA.GLOBAL_VARIABLES.VARIABLE_VALUE
> * [new] SHOW REPLICA STATUS

Reviewed-by: Brandon Nesterenko <brandon.nesterenko@mariadb.com>
ParadoxV5 added a commit that referenced this pull request Feb 1, 2025
Resize the types and widths of SHOW REPLICA STATUS
(technically `INFORMATION_SCHEMA.SLAVE_STATUS`)
columns to better match their possible values

In case of intentionally but absurdly long lists,
text columns that list an uncapped number of elements
have expanded to accept as many bytes as we could support.

Particularly, the first-gen `Replicate_` filters were
incorrectly typed as singlular `Name()`s during MDEV-33526.
Under `Name`s’ 64-char limit, they could overflow
(read: truncate) even before their lengths got absurd.

In response to `‘MAX_SLAVE_ERRMSG’ was not declared in this scope` in
Embedded builds, a new `#ifdef HAVE_REPLICATION` guard wraps
`slave_status_info` to skip this unused data in Replication-less builds.

For testing, this commit forward-ports a modified cherry-pick of #3795
(the latter targets our oldest maintained LTS as part of MDEV-35948).

> Assert that 1st-gen `replicate_*` filter variables display
> their input – including long but reasonable lists –
> correctly (without truncation) in
> * direct SELECT
> * [semi-new] INFORMATION_SCHEMA.GLOBAL_VARIABLES.VARIABLE_VALUE
> * [new] SHOW REPLICA STATUS

Reviewed-by: Brandon Nesterenko <brandon.nesterenko@mariadb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants